GH-54920: [C++][Compute] Support unsigned indices in inverse_permutation and scatter by zanmato1984 · Pull Request #55169 · apache/arrow

zanmato1984 · 2026-10-24T06:27:02Z

Rationale for this change

inverse_permutation and scatter currently accept only signed integer indices, requiring unnecessary casts for unsigned arrays. This PR adds unsigned support while reducing generated code size.

What changes are included in this PR?

Accept signed and unsigned integer indices in both functions.
Allow unsigned inverse-permutation output types, with the default still following the input type.
Share input handling by bit width and validity generation by output width, while retaining specialized output writes.
Reduce the swizzle object's code section from 432,480 to 192,872 bytes (about 55.4% smaller, Apple Clang/O2).
Preserve signed-index checks and uint64 bounds diagnostics, and update the C++ and Python-facing documentation.

Are these changes tested?

Added unit tests for unsigned types, index bounds, sliced/chunked inputs, and validity bitmap boundaries.

Are there any user-facing changes?

Unsigned integer indices are now accepted. For example, inverse permutation of uint8 [2, null, 0, 1] returns uint8 [2, 3, 0, null].

No breaking API changes.

Was AI used for this PR?

The changes were thoroughly reviewed by a human with AI assistance.

PR code and description written by:

Human
AI

Reviewed before submission by:

Human
AI
Not reviewed

GitHub Issue: [C++][Compute] inverse_permutation and scatter should accept unsigned integers #54920

…atter Signed-off-by: Rossi Sun <zanmato1984@gmail.com>

github-actions · 2026-10-24T06:27:27Z

⚠️ GitHub issue #54920 has been automatically assigned in GitHub to PR creator.

Copilot

🟢 Approval recommended

Static review found no blocking issues in dispatch, bounds checks, or validity generation, with added tests covering key boundary cases.

No open issues 🎉

What changed in this PR

Adds unsigned integer support to C++ compute functions inverse_permutation and scatter, addressing GH-54920 while reducing generated code size.

Changes:

Accept signed and unsigned indices, and allow unsigned inverse-permutation outputs.
Share input processing and validity generation by integer width while preserving bounds checks.
Expand boundary, sliced-array, and chunked-array tests and update public documentation.

File	Description
`python/pyarrow/_compute.pyx`	Document unsigned inverse-permutation output support.
`docs/source/cpp/compute.rst`	List integer input and output support.
`cpp/src/arrow/compute/kernels/vector_swizzle.cc`	Add unsigned support and consolidate kernel implementations.
`cpp/src/arrow/compute/kernels/vector_swizzle_test.cc`	Expand integer-type and boundary-case coverage.
`cpp/src/arrow/compute/api_vector.h`	Document signed and unsigned integer support.

_{Review effort: Balanced}

pitrou · 2026-10-27T10:38:15Z

+}
+
+TEST(InversePermutation, DenseValidityBitmapBoundaries) {
+  for (int64_t length : {7, 8, 9, 63, 64, 65}) {


Can you explain what this function does? Perhaps by giving an example.

Added an example in the test. For length = 8 and missing = 3, indices [0, 1, 2, 4, 5, 6, 7] produce [0, 1, 2, null, 3, 4, 5, 6]. The selected lengths and missing positions exercise byte/word boundaries and unaligned bitmap writes.

pitrou · 2026-10-27T12:48:25Z

+  for (const auto& type : IntTypes()) {
+    auto indices = ArrayFromJSON(type, "[42, 2, null, 0, 1, 42]")->Slice(1, 4);
+    auto expected = ArrayFromJSON(type, "[2, 3, 0, null]");
+    AssertInversePermutation(indices, -1, type, expected, false);


Can we make function parameters more explicit?

Suggested change

AssertInversePermutation(indices, -1, type, expected, false);

AssertInversePermutation(indices, /*xxx=*/ -1, type, expected, /*yyy=*/ false);

Added the parameter-name comments to both calls, and checked the other new tests for similar cases.

pitrou · 2026-10-27T12:48:57Z

+          Datum(ChunkedArrayFromJSON(type, {"[]", "[" + invalid_index + "]"}))}) {
+      ASSERT_RAISES_WITH_MESSAGE(
+          IndexError, "Index error: Index out of bounds: " + invalid_index,
+          InversePermutation(indices, /*max_index=*/1, /*output_type=*/int64()));


Perhaps max_index=41 to make sure that boundary cases are treated properly?

Changed to max_index = 41, so index 42 is exactly one past the allowed maximum. The negative-index and large-uint64 checks are preserved.

pitrou · 2026-10-27T12:53:10Z

+// Keep it separate from the indexed writes, and generate whole bitmap bytes
+// instead of clearing one bit at a time for every missing destination.
+template <typename OutputCType>
+ARROW_NOINLINE Status InversePermutationValidity(KernelContext* ctx,


Return a Result<std::shared_ptr<Buffer>> here?

Changed to return Result<std::shared_ptr<Buffer>>, removing the output parameter.

pitrou · 2026-10-27T12:59:16Z

+  }
+  ARROW_ASSIGN_OR_RAISE(*validity_buf, ctx->AllocateBitmap(output_length));
+  auto* validity = (*validity_buf)->mutable_data();
+  bit_util::SetBitmap(validity, 0, offset);


AllocateBitmap should already zero-initialize the bitmap, so I don't think this is necessary.

This call sets the already-valid prefix [0, offset) to ones, rather than zeroing it. The 0 is the starting bit offset. I kept the call and added /*offset=*/0 and /*length=*/offset to make that distinction explicit.

pitrou · 2026-10-27T13:07:23Z

+    // Avoid per-block validity dispatch when every input index is valid.
+    if (!span.MayHaveNulls()) {


Instead of special-casing this, perhaps rewrite indices visiting using VisitBitRuns?

Switched to VisitBitRuns, removing the separate no-null loop. In isolated inverse-permutation benchmarks on Apple M4 with Clang/O2, the traversal change gave about 1.5–1.9× speedup with 50% random nulls, but about 2.7× runtime with strictly alternating nulls. The unified traversal therefore has a known performance trade-off.

pitrou · 2026-10-27T14:36:01Z

+      {InputType(match::Integer())}, OutputType(ResolveInversePermutationOutputType));
+  kernel.init = InversePermutationState::Init;
+  kernel.exec = [](KernelContext* ctx, const ExecSpan& span, ExecResult* result) {
+    const auto exec = GenerateInteger<UnsignedInversePermutation, void, ArrayKernelExec>(


Sounds almost like we would like a GenerateInteger that takes a transformation function for the input type?

That looks reasonable. I'll review #55291 and adapt this code to the shared generator once it lands.

Signed-off-by: Rossi Sun <zanmato1984@gmail.com>

Copilot

🟢 Approval recommended

No active review issues block the changes adding unsigned-index support to inverse_permutation and scatter.

No open issues 🎉

_{Review effort: Balanced}

Signed-off-by: Rossi Sun <zanmato1984@gmail.com>

Copilot

🟢 Approval recommended

No active review issues are reported for the changes adding unsigned integer support to inverse_permutation and scatter.

No open issues 🎉

_{Review effort: Balanced}

zanmato1984

Thanks for the detailed review and helpful suggestions! I've addressed the comments below, and I'll review #55291 as well.

zanmato1984 · 2026-10-30T22:17:15Z

+}
+
+TEST(InversePermutation, DenseValidityBitmapBoundaries) {
+  for (int64_t length : {7, 8, 9, 63, 64, 65}) {


Added an example in the test. For length = 8 and missing = 3, indices [0, 1, 2, 4, 5, 6, 7] produce [0, 1, 2, null, 3, 4, 5, 6]. The selected lengths and missing positions exercise byte/word boundaries and unaligned bitmap writes.

zanmato1984 · 2026-10-30T22:17:17Z

+  for (const auto& type : IntTypes()) {
+    auto indices = ArrayFromJSON(type, "[42, 2, null, 0, 1, 42]")->Slice(1, 4);
+    auto expected = ArrayFromJSON(type, "[2, 3, 0, null]");
+    AssertInversePermutation(indices, -1, type, expected, false);


Added the parameter-name comments to both calls, and checked the other new tests for similar cases.

zanmato1984 · 2026-10-30T22:17:19Z

+          Datum(ChunkedArrayFromJSON(type, {"[]", "[" + invalid_index + "]"}))}) {
+      ASSERT_RAISES_WITH_MESSAGE(
+          IndexError, "Index error: Index out of bounds: " + invalid_index,
+          InversePermutation(indices, /*max_index=*/1, /*output_type=*/int64()));


Changed to max_index = 41, so index 42 is exactly one past the allowed maximum. The negative-index and large-uint64 checks are preserved.

zanmato1984 · 2026-10-30T22:17:21Z

+// Keep it separate from the indexed writes, and generate whole bitmap bytes
+// instead of clearing one bit at a time for every missing destination.
+template <typename OutputCType>
+ARROW_NOINLINE Status InversePermutationValidity(KernelContext* ctx,


Changed to return Result<std::shared_ptr<Buffer>>, removing the output parameter.

zanmato1984 · 2026-10-30T22:17:22Z

+  }
+  ARROW_ASSIGN_OR_RAISE(*validity_buf, ctx->AllocateBitmap(output_length));
+  auto* validity = (*validity_buf)->mutable_data();
+  bit_util::SetBitmap(validity, 0, offset);


This call sets the already-valid prefix [0, offset) to ones, rather than zeroing it. The 0 is the starting bit offset. I kept the call and added /*offset=*/0 and /*length=*/offset to make that distinction explicit.

zanmato1984 · 2026-10-30T22:17:24Z

+    // Avoid per-block validity dispatch when every input index is valid.
+    if (!span.MayHaveNulls()) {


Switched to VisitBitRuns, removing the separate no-null loop. In isolated inverse-permutation benchmarks on Apple M4 with Clang/O2, the traversal change gave about 1.5–1.9× speedup with 50% random nulls, but about 2.7× runtime with strictly alternating nulls. The unified traversal therefore has a known performance trade-off.

zanmato1984 · 2026-10-30T22:17:25Z

+      {InputType(match::Integer())}, OutputType(ResolveInversePermutationOutputType));
+  kernel.init = InversePermutationState::Init;
+  kernel.exec = [](KernelContext* ctx, const ExecSpan& span, ExecResult* result) {
+    const auto exec = GenerateInteger<UnsignedInversePermutation, void, ArrayKernelExec>(


That looks reasonable. I'll review #55291 and adapt this code to the shared generator once it lands.

[C++][Compute] Support unsigned indices in inverse permutation and sc…

f60ba85

…atter Signed-off-by: Rossi Sun <zanmato1984@gmail.com>

github-actions Bot added Component: C++ Component: Python Component: Documentation awaiting review Awaiting review labels Oct 24, 2026

zanmato1984 marked this pull request as ready for review October 24, 2026 06:38

zanmato1984 requested review from AlenkaF, pitrou, raulcd and rok as code owners October 24, 2026 06:38

Copilot AI balanced review requested due to automatic review settings October 24, 2026 06:38

Copilot started reviewing on behalf of zanmato1984 October 24, 2026 06:38

View session

Copilot AI reviewed Oct 24, 2026

View reviewed changes

pitrou reviewed Oct 27, 2026

View reviewed changes

github-actions Bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Oct 27, 2026

pitrou mentioned this pull request Oct 27, 2026

GH-55312: [C++][Compute] More flexible kernel exec generation #55291

Open

2 of 5 tasks

[C++][Compute] Use bitmap runs for inverse permutation traversal

dfd8997

Signed-off-by: Rossi Sun <zanmato1984@gmail.com>

Copilot AI balanced review requested due to automatic review settings October 30, 2026 21:27

Copilot started reviewing on behalf of zanmato1984 October 30, 2026 21:27

View session

Copilot AI reviewed Oct 30, 2026

View reviewed changes

[C++][Compute] Refine inverse permutation helpers and tests

0b0b62c

Signed-off-by: Rossi Sun <zanmato1984@gmail.com>

Copilot AI balanced review requested due to automatic review settings October 30, 2026 22:16

Copilot started reviewing on behalf of zanmato1984 October 30, 2026 22:16

View session

Copilot AI reviewed Oct 30, 2026

View reviewed changes

zanmato1984 commented Oct 30, 2026

View reviewed changes

	AssertInversePermutation(indices, -1, type, expected, false);
	AssertInversePermutation(indices, /xxx=/ -1, type, expected, /yyy=/ false);

		// Avoid per-block validity dispatch when every input index is valid.
		if (!span.MayHaveNulls()) {

Conversation

zanmato1984 commented Oct 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Was AI used for this PR?

Uh oh!

github-actions Bot commented Oct 24, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

zanmato1984 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

zanmato1984 commented Oct 24, 2026 •

edited

Loading